feat: improve collection sidebar #1320

rpenido · 2024-09-24T15:54:16Z

Description

This PR implements the Details tab in the Collection Sidebar and change the behaviour while clicking in a Collection Card in the Library Home.

More Information

Part of:

Collections Page: Collections Sidebar #1089

Testing Instruction

Open the Library Authoring Home for a library
Add a new collection
Click on the newly created Collection Card: this should open the sidebar
Click on the Details tab
Using the sidebar, edit the collection's name. You should see the new name in the sidebar and the collection list. The Last Modified date should also be updated.
Using the sidebar, edit the Collection description. You should see the new description in the sidebar and the collection list. The Last Modified date should also be updated.
Click the Open button, and you should be redirected to the Collection Page
You can also navigate to the Collection Page using the menu from the Collection Card

Private ref: FAL-3791

openedx-webhooks · 2024-09-24T15:54:19Z

Thanks for the pull request, @rpenido!

What's next?

Please work through the following steps to get your changes ready for engineering review:

🔘 Get product approval

If you haven't already, check this list to see if your contribution needs to go through the product review process.

If it does, you'll need to submit a product proposal for your contribution, and have it reviewed by the Product Working Group.
- This process (including the steps you'll need to take) is documented here.
If it doesn't, simply proceed with the next step.

🔘 Provide context

To help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:

Dependencies

This PR must be merged before / after / at the same time as ...
Blockers

This PR is waiting for OEP-1234 to be accepted.
Timeline information

This PR must be merged by XX date because ...
Partner information

This is for a course on edx.org.
Supporting documentation
Relevant Open edX discussion forum threads

🔘 Get a green build

If one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green.

🔘 Let us know that your PR is ready for review:

Who will review my changes?

This repository is currently maintained by @openedx/2u-tnl. Tag them in a comment and let them know that your changes are ready for review.

Where can I find more information?

If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources:

When can I expect my changes to be merged?

Our goal is to get community contributions seen and reviewed as efficiently as possible.

However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:

The size and impact of the changes that it introduces
The need for product review
Maintenance status of the parent repository

💡 As a result it may take up to several weeks or months to complete a review and merge your PR.

codecov · 2024-09-24T18:03:12Z

Codecov Report

Attention: Patch coverage is 99.46524% with 1 line in your changes missing coverage. Please review.

Project coverage is 92.79%. Comparing base (95c1753) to head (766e4c8).
Report is 3 commits behind head on master.

Files with missing lines	Patch %	Lines
src/library-authoring/common/context.tsx	92.30%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1320      +/-   ##
==========================================
+ Coverage   92.72%   92.79%   +0.07%     
==========================================
  Files        1035     1037       +2     
  Lines       19231    19505     +274     
  Branches     3999     4130     +131     
==========================================
+ Hits        17831    18099     +268     
- Misses       1338     1339       +1     
- Partials       62       67       +5

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

rpenido · 2024-09-25T19:59:26Z

src/library-authoring/collections/LibraryCollectionPage.test.tsx

@@ -127,7 +127,7 @@ describe('<LibraryCollectionPage />', () => {
    expect((await screen.findAllByText(libraryTitle))[0]).toBeInTheDocument();
    expect((await screen.findAllByText(mockCollection.title))[0]).toBeInTheDocument();

-    expect(screen.getByText('This collection is currently empty.')).toBeInTheDocument();
+    expect(screen.getAllByText('This collection is currently empty.')[0]).toBeInTheDocument();


This also appears in the sidebar now

rpenido · 2024-09-25T20:03:16Z

src/library-authoring/library-sidebar/LibrarySidebar.tsx

+    <SearchContextProvider
+      extraFilter={[
+        `context_key = "${library.id}"`,
+        ...(currentCollectionHit ? [`collections.key = "${currentCollectionHit.blockId}"`] : []),
+      ]}
+      overrideQueries={{ ...(collectionQuery ? { collections: collectionQuery } : {}) }}
+    >


We need a new provider in the sidebar while showing the LibraryHome. We need to refactor this in the future.

Could you put this info in a TODO code comment please, so we don't lose track of it?

pomegranited

This is working well @rpenido ! Left some comments and suggestions, and I can do a full approval when you're ready.

pomegranited · 2024-09-26T01:24:37Z

src/library-authoring/collections/CollectionInfo.tsx

+      <Tabs
+        variant="tabs"
+        className="my-3 d-flex justify-content-around"
+        defaultActiveKey="manage"


@rpenido I know the task says:

The Collection sidebar contains two tabs: Manage and Details. Manage always opens by default when the Sidebar is first opened.

But since it also says:

Manage Tab: This will be defined in epic #1094, for now the tab just needs to exist in an empty state.

I'm wondering if Product would agree to make the Details tab shown by default for Sumac? It looks bad to have the empty Manage tab displayed by default.

Agreed!
But if we get #1299 merged, I think putting the tags here would be a small task (and I expect we can get it done before Sumac).

Yes, I've asked on #1094 (comment) about that, but I think we need a story for "Add tags to collections" and it should be easy

src/library-authoring/collections/CollectionInfoHeader.test.tsx

src/library-authoring/collections/CollectionInfo.tsx

pomegranited · 2024-09-26T01:38:18Z

src/library-authoring/LibraryAuthoringPage.test.tsx

@@ -100,7 +100,8 @@ describe('<LibraryAuthoringPage />', () => {

    // Ensure the search endpoint is called:
    // Call 1: To fetch searchable/filterable/sortable library data
-    await waitFor(() => { expect(fetchMock).toHaveFetchedTimes(1, searchEndpoint, 'post'); });
+    // TODO: Check if this is the correct number of calls
+    await waitFor(() => { expect(fetchMock).toHaveFetchedTimes(2, searchEndpoint, 'post'); });


Do we know why this endpoint is being called twice now? Can that be fixed?

I refactored a bit and fixed this, but It is called twice in production when the sidebar is open:

One call from the SearchManage to fetch the blocks, facets and collections

Another call inside the Collection Sidebar to get the block types for the selected collection

@rpenido

One call from the SearchManage to fetch the blocks, facets and collections

That call makes sense 👍

Another call inside the Collection Sidebar to get the block types for the selected collection

But this one doesn't make sense.. The SearchManager returns the block types for the current library (in multisearch query #2), so can't we use those?

I think this is already resolved, @rpenido right?

src/library-authoring/collections/CollectionDetails.test.tsx

pomegranited · 2024-09-26T01:54:26Z

src/library-authoring/collections/CollectionDetails.tsx

+      {blockTypesArray.map(({ blockType, count }) => (
+        <BlockCount
+          key={blockType}
+          label={<BlockTypeLabel type={blockType} />}


It would be really cool if these BlockTypeLabel messages supported plurals. cf https://formatjs.io/docs/react-intl/components/#formattedplural

We also have one endpoint (mock here) that returns this same data (but not regionalized). I think we should also deduplicate this.

Could we handle this in a future task?

Could we handle this in a future task?

Yep, this can totally be in a separate task -- you might have to push back on UI/UX review of this task, because it's a "nice to have" on this label, and not that simple to implement.

But that endpoint you referenced returns the allowed block types for a given content library, which I suspect is an old use case for libraries v2, since we create libraries that support all block types (with lib.type=='COMPLEX' by default), and there's nowhere in the frontend to change that. I personally don't think we should be using a REST API to tell us what different block types are called -- the app translations are enough.

Eventually we can update BlockTypeLabel so that it can load and cache the translated block names for random third-party XBlocks, but it definitely shouldn't bother using a REST API for the "standard" block types. But let's not worry about that in the coming weeks.

But that endpoint you referenced returns the allowed block types for a given content library, which I suspect is an old use case for libraries v2, since we create libraries that support all block types (with lib.type=='COMPLEX' by default)

Exactly. That endpoint should be deprecated and is not the right one to use if it's not localized.

src/library-authoring/collections/CollectionDetails.tsx

Co-authored-by: Jillian <[email protected]>

rpenido · 2024-09-26T14:57:42Z

src/library-authoring/collections/LibraryCollectionPage.test.tsx

@@ -111,8 +112,6 @@ describe('<LibraryCollectionPage />', () => {
    expect((await screen.findAllByText(libraryTitle))[0]).toBeInTheDocument();
    expect((await screen.findAllByText(mockCollection.title))[0]).toBeInTheDocument();

-    expect(screen.queryByText('This collection is currently empty.')).not.toBeInTheDocument();


We have this string on the sidebar now

src/library-authoring/collections/LibraryCollectionPage.tsx

navinkarkera · 2024-09-27T06:07:56Z

src/library-authoring/collections/LibraryCollectionPage.tsx

@@ -200,7 +204,7 @@ const LibraryCollectionPage = () => {
      extraFilter={[`context_key = "${libraryId}"`, `collections.key = "${collectionId}"`]}
      overrideQueries={{ collections: collectionQuery }}


We can override collections query to an empty object as we don't need it here.

Just a note: an empty object query returns all documents from the index. I replaced it with: { limit: 0}

pomegranited

👍 This is working great @rpenido :)
I've noted a couple of things in addition to what Navin flagged, but my suggestions don't have to block merging. Happy to re-test after changes have been addressed.

I tested this using the PR test instructions.
I read through the code
I checked for accessibility issues by using my keyboard only to navigate while testing.
Includes documentation -- code comments
User-facing strings are extracted for translation

pomegranited · 2024-09-27T06:09:05Z

src/library-authoring/collections/CollectionDetails.tsx

+  const updateMutation = useUpdateCollection(library.id, collectionId);
+  const { data: collection } = useCollection(library.id, collectionId);


I'm not thrilled that we're reading from the REST API here, while everything else reads from Meilisearch. I would much prefer we carefully designed a clean way to address caching/performance issues across the app instead, otherwise we'll be chasing caching issues everywhere. cf discussion.

But will let @bradenmacdonald weigh in on whether this matters right now or not.

I put a comment in the discussion about why I had to change it here.

@rpenido's reasoning makes sense to me and I think the general pattern is: Meilisearch for search results (keywords or faceted filtering are involved), and REST APIs for everything else. I disagree that "everything else" reads from Meilisearch - any of the sidebars should be loading their content from the various REST APIs. The "tags" widget loads from the object tags REST API, the library sidebar loads from the REST API, the editors load from the REST API, and so on. Any time you load a single "thing", it's best to use the REST API because we don't need any Meilisearch features in that case, and it is more likely to be up to date.

pomegranited · 2024-09-27T06:17:18Z

src/search-manager/data/api.ts

+export const fetchBlockTypes = async (
+  client: MeiliSearch,
+  indexName: string,
+  extraFilter?: Filter,
+): Promise<Record<string, number>> => {
+  // Convert 'extraFilter' into an array
+  const extraFilterFormatted = forceArray(extraFilter);
+
+  const { results } = await client.multiSearch({
+    queries: [{
+      indexUid: indexName,
+      facets: ['block_type'],
+      filter: extraFilterFormatted,
+      limit: 0, // We don't need any "hits" for this - just the facetDistribution
+    }],
+  });
+
+  return results[0].facetDistribution?.block_type ?? {};
+};


I thought we already had this information in the 2nd multisearch query?

frontend-app-authoring/src/search-manager/data/api.ts

Lines 255 to 266 in 95c1753

// The second query is to get the possible values for the "block types" filter

queries.push({

indexUid: indexName,

q: searchKeywords,

facets: ['block_type', 'content.problem_types'],

filter: [

...extraFilterFormatted,

// We exclude the block type filter here so we get all the other available options for it.

...tagsFilterFormatted,

],

limit: 0, // We don't need any "hits" for this - just the facetDistribution

});

When we are on the library page and open the collection sidebar, the 2nd search has the facets for all blocks in this library (not only in the collection).

src/library-authoring/common/context.tsx

ChrisChV · 2024-09-28T00:43:25Z

src/library-authoring/LibraryAuthoringPage.test.tsx

@@ -100,7 +100,8 @@ describe('<LibraryAuthoringPage />', () => {

    // Ensure the search endpoint is called:
    // Call 1: To fetch searchable/filterable/sortable library data
-    await waitFor(() => { expect(fetchMock).toHaveFetchedTimes(1, searchEndpoint, 'post'); });
+    // TODO: Check if this is the correct number of calls
+    await waitFor(() => { expect(fetchMock).toHaveFetchedTimes(2, searchEndpoint, 'post'); });


I think this is already resolved, @rpenido right?

openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Sep 24, 2024

rpenido force-pushed the rpenido/fal-3791-collecion-page-sidebar branch 4 times, most recently from 87c44fd to ae93bcf Compare September 24, 2024 17:50

rpenido force-pushed the rpenido/fal-3791-collecion-page-sidebar branch 4 times, most recently from 36cdb0b to 46a2356 Compare September 25, 2024 19:54

rpenido commented Sep 25, 2024

View reviewed changes

rpenido force-pushed the rpenido/fal-3791-collecion-page-sidebar branch from 46a2356 to dc3c85f Compare September 25, 2024 20:04

feat: improve collection sidebar

f9fa28e

rpenido force-pushed the rpenido/fal-3791-collecion-page-sidebar branch 2 times, most recently from 2705d87 to ba624c6 Compare September 26, 2024 01:32

pomegranited reviewed Sep 26, 2024

View reviewed changes

rpenido force-pushed the rpenido/fal-3791-collecion-page-sidebar branch from ba624c6 to 7ef8f14 Compare September 26, 2024 13:09

refactor: remove SearchContext from sidebar

4efe06d

rpenido force-pushed the rpenido/fal-3791-collecion-page-sidebar branch from 7ef8f14 to 4efe06d Compare September 26, 2024 13:11

rpenido added 2 commits September 26, 2024 10:38

test: change block types

d23a8a5

test: refactor getBlock test

5c52971

rpenido force-pushed the rpenido/fal-3791-collecion-page-sidebar branch from 53d05c7 to 5c52971 Compare September 26, 2024 14:21

rpenido and others added 6 commits September 26, 2024 11:22

fix: typo on test

3d37dee

Co-authored-by: Jillian <[email protected]>

feat: add comments to splice blockTypesArray code

7786528

Co-authored-by: Jillian <[email protected]>

test: improve CollectionInfoHeader tests

9690499

fix: lint

58499a5

test: fix search api call count

7cbb250

fix: lint

89f0e08

rpenido commented Sep 26, 2024

View reviewed changes

rpenido added 8 commits September 26, 2024 12:56

test: add new test for block types hook

0a0530f

fix: clean unused facet

9bf728c

test: add test to library collection card click

22d967b

fix: json format

8c10dfe

fix: hide Open button if collection home is already open

4a02120

Merge branch 'master' into rpenido/fal-3791-collecion-page-sidebar

9b4dd10

chore: trigger CI

cd3b912

refactor: stop using meilisearch to get a single collection

c370a7a

rpenido force-pushed the rpenido/fal-3791-collecion-page-sidebar branch from d3e4d3d to c370a7a Compare September 26, 2024 20:46

navinkarkera reviewed Sep 27, 2024

View reviewed changes

pomegranited approved these changes Sep 27, 2024

View reviewed changes

rpenido mentioned this pull request Sep 27, 2024

feat: collections page [FC-0062] #1281

Merged

mphilbrick211 added the FC Relates to an Axim Funded Contribution project label Sep 27, 2024

rpenido added 2 commits September 27, 2024 12:20

test: more coverage

261d2a9

refactor: refactor library collection page

e795a2b

rpenido requested a review from ChrisChV September 27, 2024 15:42

rpenido marked this pull request as ready for review September 27, 2024 15:42

rpenido requested a review from a team as a code owner September 27, 2024 15:42

docs: add docstring

ce6c7bf

ChrisChV reviewed Sep 28, 2024

View reviewed changes

style: Update src/library-authoring/common/context.tsx with a nit

766e4c8

ChrisChV approved these changes Sep 28, 2024

View reviewed changes

ChrisChV merged commit 4d67e8b into openedx:master Sep 28, 2024
7 checks passed

ChrisChV deleted the rpenido/fal-3791-collecion-page-sidebar branch September 28, 2024 02:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: improve collection sidebar #1320

feat: improve collection sidebar #1320

rpenido commented Sep 24, 2024 •

edited

Loading

openedx-webhooks commented Sep 24, 2024 •

edited

Loading

codecov bot commented Sep 24, 2024 •

edited

Loading

rpenido Sep 25, 2024

rpenido Sep 25, 2024

pomegranited Sep 27, 2024

pomegranited left a comment

pomegranited Sep 26, 2024

rpenido Sep 26, 2024

bradenmacdonald Sep 26, 2024

pomegranited Sep 26, 2024

rpenido Sep 26, 2024

pomegranited Sep 27, 2024

ChrisChV Sep 28, 2024

pomegranited Sep 26, 2024

rpenido Sep 26, 2024 •

edited

Loading

pomegranited Sep 27, 2024 •

edited

Loading

bradenmacdonald Sep 27, 2024 •

edited

Loading

rpenido Sep 26, 2024

navinkarkera Sep 27, 2024

rpenido Sep 27, 2024

pomegranited left a comment

pomegranited Sep 27, 2024

rpenido Sep 27, 2024

bradenmacdonald Sep 27, 2024

pomegranited Sep 27, 2024

rpenido Sep 27, 2024

ChrisChV Sep 28, 2024

		@@ -200,7 +204,7 @@ const LibraryCollectionPage = () => {
		extraFilter={[`context_key = "${libraryId}"`, `collections.key = "${collectionId}"`]}
		overrideQueries={{ collections: collectionQuery }}

		const updateMutation = useUpdateCollection(library.id, collectionId);
		const { data: collection } = useCollection(library.id, collectionId);

	// The second query is to get the possible values for the "block types" filter
	queries.push({
	indexUid: indexName,
	q: searchKeywords,
	facets: ['block_type', 'content.problem_types'],
	filter: [
	...extraFilterFormatted,
	// We exclude the block type filter here so we get all the other available options for it.
	...tagsFilterFormatted,
	],
	limit: 0, // We don't need any "hits" for this - just the facetDistribution
	});

feat: improve collection sidebar #1320

feat: improve collection sidebar #1320

Conversation

rpenido commented Sep 24, 2024 • edited Loading

Description

More Information

Testing Instruction

openedx-webhooks commented Sep 24, 2024 • edited Loading

What's next?

🔘 Get product approval

🔘 Provide context

🔘 Get a green build

🔘 Let us know that your PR is ready for review:

Who will review my changes?

Where can I find more information?

When can I expect my changes to be merged?

codecov bot commented Sep 24, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pomegranited left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rpenido Sep 26, 2024 • edited Loading

Choose a reason for hiding this comment

pomegranited Sep 27, 2024 • edited Loading

Choose a reason for hiding this comment

bradenmacdonald Sep 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pomegranited left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rpenido commented Sep 24, 2024 •

edited

Loading

openedx-webhooks commented Sep 24, 2024 •

edited

Loading

codecov bot commented Sep 24, 2024 •

edited

Loading

rpenido Sep 26, 2024 •

edited

Loading

pomegranited Sep 27, 2024 •

edited

Loading

bradenmacdonald Sep 27, 2024 •

edited

Loading